-
Notifications
You must be signed in to change notification settings - Fork 1.4k
refactor: To use @rocket.chat/rest-typings as the single source of truth for REST API type definitions. #6876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughReplaced many local REST typing definitions with re-exports from Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (3)app/views/ReadReceiptView/index.tsx (1)
app/views/UserNotificationPreferencesView/index.tsx (2)
app/lib/methods/getSingleMessage.ts (1)
🔇 Additional comments (20)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
39693ae to
1c1e6dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/verify-migration-phase.sh (1)
1-52: Consider adding test execution to match documentation.MIGRATION_SUMMARY.md and MIGRATION_CHECKLIST.md both reference running
yarn testas part of the verification process, but this script only checks TypeScript compilation and linting. Consider adding test execution or updating the documentation to reflect the actual verification steps.🔎 Proposed addition of test execution
echo "✅ Linting passed with 0 errors" +# Run tests +echo "🧪 Running tests..." +if ! yarn test; then + echo "❌ Tests failed!" + exit 1 +fi +echo "✅ Tests passed" + echo "" echo "✅ Phase $PHASE verification complete!" echo " - TypeScript: ✅" echo " - Linting: ✅ (0 errors)" +echo " - Tests: ✅"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.gitignoreMIGRATION_CHECKLIST.mdMIGRATION_PLAN.mdMIGRATION_SUMMARY.mdapp/definitions/rest/COMPATIBILITY_LAYER.mdapp/definitions/rest/helpers/PaginatedRequest.tsapp/definitions/rest/helpers/PaginatedResult.tsapp/definitions/rest/helpers/index.tspackage.jsonscripts/verify-migration-phase.sh
🔇 Additional comments (8)
app/definitions/rest/COMPATIBILITY_LAYER.md (1)
1-52: Documentation looks good, but note the .gitignore conflict.The compatibility layer documentation is well-structured and provides clear guidance with helpful examples. However, this file is listed in
.gitignore(line 92), which conflicts with it being committed. Please refer to the review comment on.gitignorefor resolution.MIGRATION_PLAN.md (1)
1-315: Comprehensive migration plan, but conflicts with .gitignore.The migration plan is well-structured with clear phases, time estimates, and quality gates. However, this file matches the
*MIGRATION*.mdpattern in.gitignore(line 88), which would prevent it from being tracked. Please refer to the review comment on.gitignorefor resolution.app/definitions/rest/helpers/PaginatedResult.ts (1)
1-2: No action needed—verification confirms the external type exists and all usages are type-safe.The
@rocket.chat/rest-typingspackage (v6.13.1) is properly declared in package.json, and the re-export is working correctly across all seven files that importPaginatedResult(directory.ts, settings.ts, teams.ts, videoConference.ts, omnichannel.ts, emojiCustom.ts, chat.ts). No type errors or import failures are present, confirming structure compatibility and type-safety.package.json (1)
50-53: The specified versions (6.13.1) exist, are published, and have no known security vulnerabilities. They are compatible by design, released together as matched versions in Rocket.Chat 6.13.1. The exact version pinning indicates intentional selection for stable compatibility with a specific Rocket.Chat server release, which is appropriate for a mobile client targeting that version. No action needed.app/definitions/rest/helpers/index.ts (1)
2-4: LGTM! Clear migration plan documented.The TODO comments effectively communicate the migration strategy and rationale for maintaining the current implementation during the transition phase.
MIGRATION_SUMMARY.md (1)
1-58: Well-structured migration documentation.The summary provides a clear overview of the migration process with helpful quick references, phase descriptions, quality gates, and important notes. The structure will guide developers effectively through the phased migration.
MIGRATION_CHECKLIST.md (1)
1-288: Excellent comprehensive migration checklist.This checklist is well-structured and provides clear, actionable steps for each migration phase. The inclusion of verification commands (
yarn lint,tsc --noEmit) after each category and standardized commit messages will help maintain quality and consistency throughout the migration process.app/definitions/rest/helpers/PaginatedRequest.ts (1)
1-2: No action needed. The re-export to@rocket.chat/rest-typingsis confirmed compatible.The migration refactored the local
PaginatedRequestdefinition to use the external package type, which correctly supports generic parametersT(request body) and an optionalS(sort field) as evidenced by usage patterns throughout the codebase (PaginatedRequest<{ query: string }, 'name'>,PaginatedRequest<{ text: string }>). The external type includes the expected fields:count,offset,sort, andquery. The codebase actively uses this re-export without type errors across multiple endpoint definitions.
861a4fa to
d9b821b
Compare
diegolmello
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
Left a few comments.
| @@ -1,4 +1,7 @@ | |||
| /* eslint-disable @typescript-eslint/no-unused-vars */ | |||
| // TODO: Once we migrate all endpoints to use @rocket.chat/rest-typings, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do it on this PR or start the work on this PR, so it serves as a POC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure...will try my best...
Preparation & Setup - Install @rocket.chat/[email protected] - Install @rocket.chat/[email protected] (required dependency) - Explore and document package structure Helper Types Migration - Migrate PaginatedRequest to re-export from @rocket.chat/rest-typings - Migrate PaginatedResult to re-export from @rocket.chat/rest-typings - Update helpers/index.ts with TODO for future endpoint migration - Keep custom ResultFor, Serialized, ErrorResult for SDK compatibility
- Remove PaginatedRequest.ts and PaginatedResult.ts wrapper files
- Use KeyOfEach and ReplacePlaceholders from @rocket.chat packages - Implement rest-typings patterns: PathFor, PathWithParamsFor, PathWithoutParamsFor - Add utilities: JoinPathPattern, UrlParams, MethodOf, ParamsFor - Use MethodToPathPatternToParamsMap and MethodToPathPatternToResultMap patterns - Fix TypeScript errors in useEndpointData with proper type guards - Update import order in endpoint definition files
0aa5c81 to
b5e17a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/definitions/rest/helpers/index.ts (2)
1-1: Verify the necessity of the eslint-disable directive.The
no-unused-varsdisable is common in type-only files, but it can mask legitimate issues. Consider reviewing whether all type parameters are actually necessary.
2-4: Track the migration TODO for future completion.The POC comment indicates this is a temporary bridge implementation until full migration to
@rocket.chat/rest-typings. This technical debt should be tracked to ensure the migration is completed.Would you like me to create a tracking issue for the full migration to
@rocket.chat/rest-typingsendpoints (replacing the current Endpoints structure with/v1/prefixed paths)?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
app/definitions/rest/helpers/PaginatedRequest.tsapp/definitions/rest/helpers/PaginatedResult.tsapp/definitions/rest/helpers/index.tsapp/definitions/rest/v1/channels.tsapp/definitions/rest/v1/chat.tsapp/definitions/rest/v1/directory.tsapp/definitions/rest/v1/emojiCustom.tsapp/definitions/rest/v1/groups.tsapp/definitions/rest/v1/im.tsapp/definitions/rest/v1/omnichannel.tsapp/definitions/rest/v1/settings.tsapp/definitions/rest/v1/teams.tsapp/definitions/rest/v1/videoConference.tsapp/lib/hooks/useEndpointData.tspackage.json
💤 Files with no reviewable changes (2)
- app/definitions/rest/helpers/PaginatedRequest.ts
- app/definitions/rest/helpers/PaginatedResult.ts
✅ Files skipped from review due to trivial changes (1)
- app/definitions/rest/v1/videoConference.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
app/definitions/rest/helpers/index.ts (1)
app/definitions/rest/v1/index.ts (1)
Endpoints(25-47)
app/lib/hooks/useEndpointData.ts (1)
app/definitions/rest/helpers/index.ts (4)
ErrorResult(182-182)Serialized(48-56)ResultFor(177-180)MatchPathPattern(59-61)
🔇 Additional comments (13)
app/lib/hooks/useEndpointData.ts (1)
45-51: The error-handling logic is correct and intentional.The SDK resolves promises in both success and error cases—it only rejects for network/connection errors. The logic correctly identifies errors when responses have
success: falseand treats all other resolved values as results. This matches the actual SDK behavior shown in the tests and is part of the@rocket.chat/rest-typingsmigration.The guards are appropriate:
e && typeof e === 'object' && 'success' in e && !e.successensures non-objects and objects without a success property pass through tosetResult, which is the correct behavior since the type system guarantees all SDK responses are objects with a success property.Likely an incorrect or invalid review comment.
app/definitions/rest/v1/im.ts (1)
1-1: LGTM! Import migration correctly applied.The migration from local
PaginatedRequestto@rocket.chat/rest-typingsfollows the same pattern as other files in this PR. Type-only import is appropriate.app/definitions/rest/v1/channels.ts (1)
1-1: LGTM! Consistent with the rest-typings migration.The migration from local
PaginatedRequestto@rocket.chat/rest-typingsis correctly implemented.app/definitions/rest/v1/teams.ts (1)
1-1: LGTM! Migration correctly applied.The migration from local
PaginatedResultto@rocket.chat/rest-typingsis consistent with the broader refactoring effort.app/definitions/rest/v1/settings.ts (1)
1-1: LGTM! Import migration correctly applied.The migration from local
PaginatedResultto@rocket.chat/rest-typingsis correctly implemented. The type is used in an intersection type at line 70, which should work seamlessly with the external definition.app/definitions/rest/v1/chat.ts (1)
1-1: LGTM! Migration to @rocket.chat/rest-typings is correctly implemented.The type-only import from
@rocket.chat/rest-typingsforPaginatedResultis properly used and compatible. The old local definition has been removed, and the genericPaginatedResult<T>type matches the usage patterns in chat.ts and other REST endpoint definitions. Package version 6.13.1 is stable and was released as part of the official Rocket.Chat 6.13.1 release.app/definitions/rest/v1/emojiCustom.ts (1)
1-1: LGTM! Clean migration to external types.The import consolidation from local helpers to
@rocket.chat/rest-typingsaligns with the PR objectives and maintains existing type signatures.app/definitions/rest/v1/directory.ts (1)
1-1: LGTM! Consistent with migration pattern.The import source change maintains the existing type contract while aligning with the centralized type definitions from
@rocket.chat/rest-typings.app/definitions/rest/v1/omnichannel.ts (1)
1-1: LGTM! Extensive migration completed successfully.The import migration to
@rocket.chat/rest-typingsis applied consistently across all OmnichannelEndpoints, maintaining type safety while reducing local type duplication.app/definitions/rest/helpers/index.ts (3)
29-29: Type intersection ensures string compatibility.The
TPathPattern & stringintersection correctly constrains the type forReplacePlaceholders, ensuring that only string-compatible path patterns are processed.
175-180: LGTM! Error handling wrapper aligns with SDK needs.The
ResultFortype correctly wraps operation results withSuccessResult,FailureResult, andUnauthorizedResultpatterns, providing comprehensive error handling for SDK consumers.
64-158: This is a POC adaptation—verify against actual rest-typings 6.13.1 when migrating endpoints.The type utilities are correctly documented as adaptations rather than direct implementations from
@rocket.chat/rest-typings. The file comment explicitly notes this is a temporary POC pattern awaiting full endpoint migration. The only direct import from rest-typings isReplacePlaceholders, which is properly re-exported; the remaining utilities (UrlParams,MethodOf,PathFor, etc.) are locally implemented to match the current Endpoints structure.Before merging, confirm:
- The local type implementations are compatible with rest-typings 6.13.1's type signatures when endpoints are eventually migrated to use
/v1/prefix paths- No breaking changes in future rest-typings versions (6.13.1+) will affect the mapped type patterns used here
The TypeScript strict mode configuration and tsc in the lint script will catch type errors during compilation, but real compatibility should be verified once these types are actively used.
app/definitions/rest/v1/groups.ts (1)
1-1: Clean refactor with confirmed package compatibility.The migration to
@rocket.chat/rest-typingsimproves type consistency across the codebase. The package version6.13.1is correctly specified inpackage.json, andPaginatedRequestis properly exported and already in use across multiple endpoint definitions.
- Migrate subscriptions, push, customUserStatus, directory, permissions, and roles endpoints - Use type adapters to remove /v1/ prefix for compatibility with current path structure - Fix type errors in DirectoryView and restApi.ts - Update toggleReadStatus to use explicit endpoint types - Fix push token type to use 'apn' | 'gcm' instead of string - Add local extensions for roles.create, roles.update, and roles.getUsersInPublicRoles
…/rest-typings - Migrate users, rooms, chat, channels, groups, im, dm, teams, emojiCustom, invites, and omnichannel endpoints - Add type assertions where needed for compatibility with rest-typings types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/services/restApi.ts (1)
333-346: Fix inconsistent payload field naming intoggleReadStatus.The function uses different field names for the room identifier between endpoints:
subscriptions.unreadreceives{ roomId }subscriptions.readreceives{ rid: roomId }The
subscriptions.readendpoint pattern is confirmed byapp/lib/methods/readMessages.ts(line 37), which also uses{ rid }. To maintain consistency,subscriptions.unreadshould useridinstead ofroomId.Additionally, the return type
Promise<any>loses type safety. Consider defining the actual response type from rest-typings or adding a TODO comment to restore typing.
🤖 Fix all issues with AI agents
In `@app/lib/methods/getSingleMessage.ts`:
- Around line 8-9: getSingleMessage currently uses an unsafe type assertion "as
IMessage" hiding a structural mismatch (response has _id while IMessage expects
id); update getSingleMessage to either map the sdk.get('chat.getMessage') result
into the correct shape by copying _id to id before resolving (preserve all other
fields) or change the function return type to the raw response type from
rest-typings and remove the assertion so callers like getMessageInfo and
getThreadName explicitly handle the _id→id mapping; locate getSingleMessage and
the sdk.get('chat.getMessage') call to implement the mapping or adjust the
return type accordingly.
In `@app/lib/services/restApi.ts`:
- Around line 359-361: The getUserPreferences function is incorrectly passing a
userId to sdk.get for the 'users.getPreferences' endpoint which accepts no
parameters; update the function signature remove the userId argument, remove the
`{ userId } as any` payload, and call sdk.get('users.getPreferences') with no
second argument inside getUserPreferences; if you actually need another user's
preferences, replace the call with the correct endpoint/mechanism instead of
passing userId to 'users.getPreferences'.
In `@app/views/DiscussionsView/index.tsx`:
- Around line 67-73: The code casts result to any which removes type safety;
instead type the response returned by the API (avoid "as any") by
importing/using the appropriate type from `@rocket.chat/rest-typings` or declare a
local interface matching { messages?: IMessageFromServer[]; count?: number;
total?: number } and use a type guard/adapter to convert/validate result before
using it; then replace uses of || with nullish coalescing (use offset.current +=
resultData.count ?? 0 and total.current = resultData.total ?? 0) and keep the
existing logic around isSearching/setSearch/setDiscussions but operate on the
properly typed resultData.
In `@app/views/ReadReceiptView/index.tsx`:
- Line 90: The current cast "result.receipts as any" in ReadReceiptView
(index.tsx) bypasses TS safety; replace the cast by aligning the response type
from chat.getMessageReadReceipts with the expected receipts shape (or add a
proper adapter/mapper) and narrow the union with a type guard: inspect
result.success and then assign result.receipts to a strongly typed variable
(e.g., IReadReceipt[] or the appropriate interface from
`@rocket.chat/rest-typings`) or transform result.receipts into that interface;
update the rest-typings response type to include the receipts field if it’s
missing, or implement a local typed adapter function (e.g., mapReceiptsFromSdk)
and use that instead of casting to any.
In `@app/views/UserNotificationPreferencesView/index.tsx`:
- Line 49: The code is using an unsafe cast (result.preferences as any) which
bypasses typing; replace this by aligning types: import the preferences type
from `@rocket.chat/rest-typings` and use it for the local INotificationPreferences
(or update INotificationPreferences to match that imported type), change the
setPreferences call to accept the correctly typed value, and remove the other as
any in getUserPreferences in restApi.ts by updating that function to return the
proper typed response or add a small adapter function (e.g.,
transformUserPreferences) to convert the SDK shape into INotificationPreferences
before calling setPreferences.
♻️ Duplicate comments (2)
app/views/RoomView/RightButtons.tsx (1)
280-281: Duplicateas anycast pattern.This is the same type-unsafe pattern as in
Livechat.tsx. Once you extract a shared typed helper (as suggested in the other file), apply it here as well to maintain consistency and type safety across the codebase.app/views/RoomActionsView/index.tsx (1)
411-412: Duplicateas anycast pattern.Same type-unsafe workaround for department info. Consolidate with the shared helper suggested for
Livechat.tsx.
🧹 Nitpick comments (18)
app/views/RoomInfoView/index.tsx (1)
171-172: Avoidas anycasts; use proper type alignment instead.These
as anyassertions undermine the type safety benefits of migrating to@rocket.chat/rest-typings. If there's a mismatch between the REST response type and localIUser, consider:
- Using a more specific type assertion (e.g.,
as Pick<IUser, 'username' | 'roles'>)- Creating a type adapter function that maps the REST response to the expected local type
- Extending/aligning the local types with rest-typings
Note the inconsistency: the
elsebranch (lines 178-179) handlesroomUserwithout any casts, suggesting the types should be compatible.♻️ Suggested approach
if (result.success) { const { user } = result; - const r = handleRoles(user as any); - setRoomUser({ ...user, roles: r } as any); + const r = handleRoles(user as Pick<IUser, 'username' | 'roles'>); + setRoomUser({ ...user, roles: r } as IUser); }Or create a mapper function if the types diverge significantly:
const mapRestUserToRoomUser = (user: RestUserType): Partial<IUser> => ({ _id: user._id, username: user.username, name: user.name, roles: user.roles, // ... map other fields });app/views/CannedResponsesListView/index.tsx (2)
88-88: Double cast indicates type mismatch between rest-typings and local definition.The
as unknown as ILivechatDepartment[]pattern is a valid escape hatch for the migration, but it signals that the@rocket.chat/rest-typingsdepartment type differs from the localILivechatDepartmentinterface. Consider aligning the local interface with the external type definition in a follow-up to eliminate this cast.
130-130: Prefer double cast overas anyfor type safety.Casting to
anycompletely disables type checking on the state update. For consistency with line 88 and to retain some type safety, consider using the double cast pattern instead:♻️ Suggested improvement
- setCannedResponses(prevCanned => (debounced ? res.cannedResponses : [...prevCanned, ...res.cannedResponses]) as any); + setCannedResponses(prevCanned => + (debounced ? res.cannedResponses : [...prevCanned, ...res.cannedResponses]) as unknown as ICannedResponse[] + );This maintains the type escape hatch needed for the migration while preserving the declared state type.
app/lib/methods/getCustomEmojis.ts (1)
140-142: Unsafe type assertion masks a type mismatch.The
as unknown as TCustomEmojiModel[]cast bypasses type safety entirely. The API returns raw emoji objects (with_id), whileTCustomEmojiModelis a WatermelonDBModeltype (withidand Model methods). This mismatch is evident inupdateEmojiswhereupdateitems are accessed via._idbutallRecordsitems via.id.Consider updating the
IUpdateEmojisinterface to use the correct API response type (e.g., from@rocket.chat/rest-typings) forupdateandremoveinstead ofTCustomEmojiModel[]:♻️ Suggested approach
+import type { ICustomEmoji } from '@rocket.chat/core-typings'; // or appropriate type from rest-typings interface IUpdateEmojis { - update: TCustomEmojiModel[]; - remove?: TCustomEmojiModel[]; + update: ICustomEmoji[]; + remove?: ICustomEmoji[]; allRecords: TCustomEmojiModel[]; }Then the call site would not need the unsafe cast:
-const changedEmojis = await updateEmojis({ update: (update || []) as unknown as TCustomEmojiModel[], remove: (remove || []) as unknown as TCustomEmojiModel[], allRecords }); +const changedEmojis = await updateEmojis({ update: update || [], remove: remove || [], allRecords });app/views/ThreadMessagesView/index.tsx (2)
345-351: Prefer proper type adapters overas anycasts.The
as anycasts bypass TypeScript's type safety entirely, which undermines the goal of using@rocket.chat/rest-typingsas the source of truth. If the types from the external package don't match the internalIMessagetype expected byupdateThreads, consider:
- Creating a type adapter function to map the external thread type to
IMessage- Using a more specific type assertion if the types are compatible
- Updating
updateThreadsto accept the external type directlyThe fallback
resultData.count || result.threads.lengthsuggests the response shape is uncertain—ifcountexists on the actual API response but not in the types, consider extending the type definition or using a type guard.Example type adapter approach
// Define adapter if types differ structurally type ThreadsListResponse = Awaited<ReturnType<typeof getThreadsList>>; const mapThreadsToMessages = (threads: ThreadsListResponse['threads']): IMessage[] => { return threads.map(thread => buildMessage(thread) as IMessage); }; // Then use: this.updateThreads({ update: mapThreadsToMessages(result.threads), lastThreadSync });For the
countproperty, if it's a known part of the API response:// Use type assertion with a specific interface interface ThreadsListResultWithCount { count?: number; threads: { length: number }; } const resultData = result as ThreadsListResultWithCount;
370-370: Sameas anypattern as inload—apply consistent type handling.This cast has the same type safety concerns noted above. When refactoring the
loadmethod, apply the same adapter pattern here for consistency.app/ee/omnichannel/lib/subscriptions/inquiry.ts (1)
93-94: Consider fixing the type at the source instead of inline annotations.The inline type annotations on lines 93–94 suggest that
departmentsis loosely typed after the SDK call. The root cause is theas anycast on thesdk.get()URL ingetAgentDepartments, which prevents TypeScript from inferring the response shape. While the annotations work, they:
- Add verbosity that wouldn't be needed if
getAgentDepartmentsexplicitly returned a typed response- Could mask type mismatches if the actual API response differs from the assumed
{ departmentId: string }shapeInstead of adding inline type guards, either remove the
as anycast from the SDK call or add an explicit return type togetAgentDepartmentsusing the existingOmnichannelEndpointstype from@rocket.chat/rest-typings. This would allow TypeScript to automatically infer the correct types downstream.app/views/ForwardLivechatView.tsx (1)
79-88: Double cast bypasses type safety and masks API response structure mismatch.The
as unknown as IServerRoompattern silences the compiler but doesn't guarantee that the actualrooms.infoAPI response includes all properties required byIServerRoom(e.g.,u,uids,usersCount). If the API returns a partial room object, property accesses could fail at runtime.The root issue is that
sdk.get('rooms.info', { roomId })has no explicit return type. Consider:
- Type the
getRoomInfocall using the response type from@rocket.chat/rest-typings- Create a mapper function that extracts and validates only the properties actually needed (
servedBy,departmentId)- Replace the local
IServerRoomwith the canonical type from the external packageapp/views/MessagesView/index.tsx (1)
190-190: Doubleas anycast masks type incompatibility betweenrest-typingsandIServerAttachment[].The input and output casts to
anysuppress TypeScript checking for this entire expression. Sincesdk.get()is strongly typed viarest-typingsgenerics,result.fileshas a defined type that doesn't match whatEncryption.decryptFilesexpects. Rather than silencing the compiler:
- Check the actual type that
rest-typingsdefines for the files endpoint response- Update
IServerAttachment[]to match, or add a type adapter ifrest-typingshas a different file structure- Ensure
decryptFilesproperly types its return value instead of falling back toanyapp/views/RoomInfoView/Livechat.tsx (1)
35-36: Avoidas anycast; extract a typed helper instead.Casting
result as anydefeats the purpose of adopting typed REST definitions. This same pattern appears in at least three files (Livechat.tsx,RightButtons.tsx,RoomActionsView/index.tsx), which suggests creating a shared utility.Consider narrowing the type or adding a type guard:
♻️ Suggested approach
Create a helper in the REST API layer that properly types the department response:
// In app/lib/services/restApi.ts or a dedicated helper export const getDepartmentFromResult = ( result: Awaited<ReturnType<typeof getDepartmentInfo>> ): ILivechatDepartment | undefined => { if (result.success && 'department' in result) { return result.department as ILivechatDepartment; } return undefined; };Then usage becomes type-safe:
-const resultData = result as any; -setDepartment(resultData.department as ILivechatDepartment); +const dept = getDepartmentFromResult(result); +if (dept) setDepartment(dept);app/definitions/rest/v1/invites.ts (1)
1-9: Consider extractingRemoveV1Prefixto a shared utility.The
RemoveV1Prefixtype is duplicated across multiple endpoint definition files (channels.ts,chat.ts,dm.ts,groups.ts,im.ts,omnichannel.ts,rooms.ts,invites.ts, etc.). Extract it to a common location to reduce duplication.♻️ Suggested refactor
Create a shared utility file:
// app/definitions/rest/v1/helpers.ts export type RemoveV1Prefix<T> = T extends `/v1/${infer Rest}` ? Rest : T; export type AdaptEndpoints<T> = { [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; };Then each endpoint file becomes simpler:
-import type { InvitesEndpoints as RestTypingsInvitesEndpoints } from '@rocket.chat/rest-typings'; - -type RemoveV1Prefix<T> = T extends `/v1/${infer Rest}` ? Rest : T; - -type AdaptInvitesEndpoints<T> = { - [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; -}; - -export type InvitesEndpoints = AdaptInvitesEndpoints<RestTypingsInvitesEndpoints>; +import type { InvitesEndpoints as RestTypingsInvitesEndpoints } from '@rocket.chat/rest-typings'; +import type { AdaptEndpoints } from './helpers'; + +export type InvitesEndpoints = AdaptEndpoints<RestTypingsInvitesEndpoints>;app/definitions/rest/v1/channels.ts (1)
1-9: Transformation approach is correct; consider extracting shared utility types.The type transformation correctly strips the
/v1/prefix from endpoint keys. However,RemoveV1Prefix<T>and theAdapt*Endpoints<T>pattern are duplicated across multiple files (channels, chat, users, dm, im, etc.).Consider extracting these to a shared utility file to reduce duplication:
♻️ Suggested shared utility
Create a shared utility file (e.g.,
app/definitions/rest/v1/helpers.ts):export type RemoveV1Prefix<T> = T extends `/v1/${infer Rest}` ? Rest : T; export type AdaptEndpoints<T> = { [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; };Then simplify each endpoint file:
-import type { ChannelsEndpoints as RestTypingsChannelsEndpoints } from '@rocket.chat/rest-typings'; - -type RemoveV1Prefix<T> = T extends `/v1/${infer Rest}` ? Rest : T; - -type AdaptChannelsEndpoints<T> = { - [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; -}; - -export type ChannelsEndpoints = AdaptChannelsEndpoints<RestTypingsChannelsEndpoints>; +import type { ChannelsEndpoints as RestTypingsChannelsEndpoints } from '@rocket.chat/rest-typings'; +import type { AdaptEndpoints } from './helpers'; + +export type ChannelsEndpoints = AdaptEndpoints<RestTypingsChannelsEndpoints>;app/definitions/rest/v1/rooms.ts (1)
1-9: Consider extractingRemoveV1Prefixto a shared utility.The implementation is correct and the approach of deriving endpoint types from
@rocket.chat/rest-typingsis sound. However,RemoveV1Prefix<T>is duplicated across multiple files (rooms.ts, teams.ts, emojiCustom.ts, omnichannel.ts, and likely others such as channels.ts, chat.ts, groups.ts per the AI summary).Extract this helper to a shared location to reduce duplication and simplify future maintenance.
Example shared utility file
Create a file like
app/definitions/rest/v1/helpers.ts:export type RemoveV1Prefix<T> = T extends `/v1/${infer Rest}` ? Rest : T; export type AdaptEndpoints<T> = { [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; };Then in each endpoint file:
-type RemoveV1Prefix<T> = T extends `/v1/${infer Rest}` ? Rest : T; - -type AdaptRoomsEndpoints<T> = { - [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; -}; - -export type RoomsEndpoints = AdaptRoomsEndpoints<RestTypingsRoomsEndpoints>; +import type { AdaptEndpoints } from './helpers'; + +export type RoomsEndpoints = AdaptEndpoints<RestTypingsRoomsEndpoints>;app/lib/services/restApi.ts (4)
260-260: Consider documenting whyas anyis needed here.The conditional param structure (different shapes for channels vs groups, and version-dependent fields) likely doesn't align with the strict rest-typings definitions. This cast is acceptable for now, but consider adding a brief comment explaining the type mismatch or filing a follow-up to properly type this once rest-typings supports the variant shapes.
425-434: Document that this endpoint may not be in rest-typings yet.The
livechat/inquiries.returnAsInquiryendpoint (RC 7.12.0+) requiresas anycasts, likely because it's not yet defined in the installed@rocket.chat/rest-typingsversion. Consider adding a comment or TODO to remove the casts when rest-typings is updated.
488-490: Consider separating query params from the URL path.The query parameter
enabledDepartmentsOnly=trueis embedded in the URL string. For consistency with other SDK calls and better type support in the future, consider passing it as a params object if the SDK supports it:sdk.get(`livechat/agents/${uid}/departments`, { enabledDepartmentsOnly: true })
965-968: Clarify the endpoint version and return type.The
// RC ?comment indicates uncertainty about when this endpoint was introduced. The return typePromise<boolean | {}>is also unusual. Consider:
- Documenting the actual RC version if known
- Refining the return type to match the actual API response
app/lib/methods/getSingleMessage.ts (1)
4-15: Consider refactoring to avoid the Promise constructor anti-pattern.Wrapping
asyncinsidenew Promise()is redundant and can obscure errors. This is pre-existing code, but a future cleanup could simplify it:♻️ Suggested refactor
-const getSingleMessage = (messageId: string): Promise<IMessage> => - new Promise(async (resolve, reject) => { - try { - const result = await getSingleMessageService(messageId); - if (result.success && result.message) { - return resolve(result.message as IMessage); - } - return reject(); - } catch (e) { - return reject(e); - } - }); +const getSingleMessage = async (messageId: string): Promise<IMessage> => { + const result = await getSingleMessageService(messageId); + if (result.success && result.message) { + return result.message as IMessage; + } + throw new Error('Failed to fetch message'); +};
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (31)
app/definitions/rest/v1/channels.tsapp/definitions/rest/v1/chat.tsapp/definitions/rest/v1/dm.tsapp/definitions/rest/v1/emojiCustom.tsapp/definitions/rest/v1/groups.tsapp/definitions/rest/v1/im.tsapp/definitions/rest/v1/invites.tsapp/definitions/rest/v1/omnichannel.tsapp/definitions/rest/v1/rooms.tsapp/definitions/rest/v1/teams.tsapp/definitions/rest/v1/users.tsapp/ee/omnichannel/lib/subscriptions/inquiry.tsapp/lib/hooks/useUserData.tsapp/lib/methods/getCustomEmojis.tsapp/lib/methods/getRoomInfo.tsapp/lib/methods/getSingleMessage.tsapp/lib/services/restApi.tsapp/views/CannedResponsesListView/index.tsxapp/views/DiscussionsView/index.tsxapp/views/ForwardLivechatView.tsxapp/views/LivechatEditView.tsxapp/views/MessagesView/index.tsxapp/views/ReadReceiptView/index.tsxapp/views/RoomActionsView/index.tsxapp/views/RoomInfoView/Livechat.tsxapp/views/RoomInfoView/index.tsxapp/views/RoomView/RightButtons.tsxapp/views/SearchMessagesView/index.tsxapp/views/TeamChannelsView.tsxapp/views/ThreadMessagesView/index.tsxapp/views/UserNotificationPreferencesView/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/definitions/rest/v1/groups.ts
🧰 Additional context used
🧬 Code graph analysis (10)
app/lib/methods/getSingleMessage.ts (1)
app/definitions/IMessage.ts (1)
IMessage(147-178)
app/views/ForwardLivechatView.tsx (1)
app/definitions/IRoom.ts (1)
IServerRoom(152-242)
app/views/RoomInfoView/Livechat.tsx (1)
app/definitions/ILivechatDepartment.ts (1)
ILivechatDepartment(1-23)
app/views/SearchMessagesView/index.tsx (1)
app/definitions/IMessage.ts (2)
IMessageFromServer(109-137)TMessageModel(180-183)
app/views/CannedResponsesListView/index.tsx (1)
app/definitions/ILivechatDepartment.ts (1)
ILivechatDepartment(1-23)
app/lib/services/restApi.ts (1)
app/lib/notifications/index.ts (1)
getDeviceToken(83-83)
app/views/RoomActionsView/index.tsx (1)
app/definitions/ILivechatDepartment.ts (1)
ILivechatDepartment(1-23)
app/lib/methods/getRoomInfo.ts (1)
app/definitions/IRoom.ts (1)
RoomType(147-147)
app/views/RoomView/RightButtons.tsx (1)
app/definitions/ILivechatDepartment.ts (1)
ILivechatDepartment(1-23)
app/lib/methods/getCustomEmojis.ts (1)
app/definitions/IEmoji.ts (1)
TCustomEmojiModel(35-35)
🔇 Additional comments (29)
app/lib/methods/getRoomInfo.ts (2)
25-25: LGTM! Good defensive null check.Adding the explicit
result.roomguard is a sensible improvement that prevents potential runtime errors if the API response lacks theroomproperty despitesuccess: true.
30-30: Type cast is consistent with existing pattern.The
as RoomTypeassertion aligns with line 20 and is a reasonable adaptation for the rest-typings migration where the external package may define a broader type for thetproperty.app/views/SearchMessagesView/index.tsx (1)
182-182: LGTM!The
|| []fallback correctly handles the case whereresult.messagesis undefined (from the optional chaining on line 164). The type assertion to(IMessageFromServer | TMessageModel)[]is appropriate here since the method needs to return a union type that accommodates both server responses and local WatermelonDB models returned from the encrypted room branch.app/views/TeamChannelsView.tsx (1)
336-363: Good defensive check, but verify the empty params fallback behavior.The added check for
result.roomis a sound improvement that prevents accessing properties on a potentially undefined object. However, when the condition fails (either!result.successor!result.room),goRoomis still invoked with an emptyparamsobject, which could lead to unexpected navigation behavior or errors downstream.Consider whether an early return or user feedback should be provided when room info cannot be retrieved:
Suggested improvement
onPressItem = debounce( async (item: IItem) => { logEvent(events.TC_GO_ROOM); const { isMasterDetail } = this.props; try { - let params = {}; const result = await getRoomInfo(item._id); - if (result.success && result.room) { - params = { - rid: item._id, - name: getRoomTitle(result.room), - joinCodeRequired: result.room.joinCodeRequired, - t: result.room.t, - teamId: result.room.teamId - }; + if (!result.success || !result.room) { + showErrorAlert(I18n.t('Room_not_found')); + return; } + const params = { + rid: item._id, + name: getRoomTitle(result.room), + joinCodeRequired: result.room.joinCodeRequired, + t: result.room.t, + teamId: result.room.teamId + }; goRoom({ item: params, isMasterDetail }); } catch (e: any) {app/lib/methods/getCustomEmojis.ts (1)
127-130: LGTM on parameter shape change.The updated parameter shape
{ query: string; updatedSince?: string }aligns with theemoji-custom.listendpoint definition from@rocket.chat/rest-typings. Initializing with{ query: '' }correctly satisfies the required parameter.app/views/LivechatEditView.tsx (1)
124-124: The endpoint's type annotation should be improved before enforcing strict types on the callback.The use of
anyon line 124 is a consequence of the endpoint being cast toas anyinapp/lib/services/restApi.ts:490. While using a proper type from@rocket.chat/rest-typingswould be ideal, the SDK cannot infer correct types when the endpoint path itself is marked asany. Consider fixing the root cause by removing or refining theas anycast on the endpoint definition first, which would then allow the callback parameter to receive proper typing through the SDK's genericResultFor<'GET', MatchPathPattern<TPath>>mechanism.app/lib/hooks/useUserData.ts (1)
35-36: LGTM!Good defensive defaults. The fallbacks ensure consistency with the initial state shape and prevent potential
undefinedpropagation to child components.app/views/RoomActionsView/index.tsx (3)
203-208: LGTM!Good use of nullish coalescing to handle potentially undefined
counters.members. The distinction between?? undefined(for state) and?? 0(for the update function) correctly preserves the semantic difference between "unknown count" and "zero members".
246-247: Consistent nullish coalescing.Same defensive pattern applied correctly in the
componentDidMountpath.
612-612: Remove the unnecessaryanycast ifisLastOwneris available from the response type, or extend the type locally if it's missing.The
(r as any).isLastOwnercast suggestsisLastOwneris not typed in theteamListRoomsOfUserresponse from the SDK (which uses@rocket.chat/rest-typings). However,isLastOwneris already defined in the localIRoominterface as an optional boolean. Compare this withRoomMembersView/helpers.ts:156, which handles the same API response by casting the entire loop variable toanyinstead of just the property. If the endpoint's type definition in rest-typings lacksisLastOwner, consider either extending the response type locally or opening an issue upstream to add it to the type definitions.app/definitions/rest/v1/chat.ts (1)
1-9: LGTM!The transformation logic is correct and follows the same pattern established in other endpoint files. The duplication concern applies here as well—once the shared utility is extracted, this file can be simplified.
app/definitions/rest/v1/users.ts (1)
1-9: LGTM!Consistent with the transformation pattern used across other endpoint definition files. The type mapping correctly adapts
RestTypingsUsersEndpointsby stripping the/v1/prefix.app/definitions/rest/v1/dm.ts (1)
1-9: LGTM!Follows the established pattern for adapting REST typings. The transformation correctly maps
RestTypingsDmEndpointstoDmEndpointswith prefix stripping.app/definitions/rest/v1/im.ts (1)
1-9: LGTM!Consistent with the transformation approach across all endpoint definition files. The migration to
@rocket.chat/rest-typingsas the single source of truth is well-structured.app/definitions/rest/v1/rooms.ts (1)
11-13: LGTM!
TRoomsMediaResponseis unchanged and remains correctly defined as a local type.app/definitions/rest/v1/teams.ts (1)
1-9: LGTM — same refactor opportunity as noted in rooms.ts.The adapter pattern is correctly implemented. The
RemoveV1PrefixandAdaptTeamsEndpointshelpers duplicate the same logic from other files and would benefit from consolidation into a shared utility.app/definitions/rest/v1/emojiCustom.ts (1)
1-9: LGTM — same refactor opportunity applies.Correctly derives
EmojiCustomEndpointsfrom the external rest-typings package. The duplicated helper types should be consolidated per the suggestion in rooms.ts.app/definitions/rest/v1/omnichannel.ts (1)
1-9: LGTM — same refactor opportunity applies.Correctly derives
OmnichannelEndpointsfrom the external rest-typings package. Consider consolidating the duplicated helper types as suggested in the rooms.ts review.app/lib/services/restApi.ts (11)
271-277: LGTM!Converting
nulltoundefinedvia?? undefinedaligns with rest-typings expectations for optional parameters.
473-478: LGTM for the routing config endpoint cast.The
livechat/config/routingendpoint (RC 7.11.0+) cast is acceptable since it's likely not yet in rest-typings.
599-601: LGTM!Converting the boolean
ignoreto string via.toString()aligns with the API's expected parameter type.
646-651: LGTM with reservation.The
as anycasts ondataandcustomFieldsbypass rest-typings validation. The local types appear reasonable, but consider verifying alignment with rest-typings definitions when refining this migration.
657-659: LGTM!
696-704: LGTM!The sort object cast is acceptable - MongoDB-style sort objects often need casting due to strict typing constraints.
797-809: LGTM!
970-991: LGTM!The dual payload structure (content vs text) for encrypted and regular messages requires casting since rest-typings likely doesn't model both variants.
993-1011: LGTM!Explicit type annotation
'apn' | 'gcm'ensures the literal union type is preserved for rest-typings compatibility.
1083-1086: LGTM!
1095-1095: LGTM!
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Fix CommandsEndpoints and AutoTranslateEndpoints imports by extracting from Endpoints interface - Fix LoginServiceConfiguration import to use @rocket.chat/core-typings - Fix runSlashCommand to make triggerId required to match endpoint definition - Fix getSlashCommands type assertions for commands.list endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/definitions/rest/v1/videoConference.ts (1)
9-13: Fix return type: endpoint returnsPromise<void>, notvoid.The
POSTmethod for this endpoint is typed incorrectly. The actual usage inapp/lib/methods/videoConfTimer.tscalls.catch()on the result ofupdateJitsiTimeout, which requires the return type to be a Promise:updateJitsiTimeout(rid).catch((e: unknown) => console.log(e));The return type should be
Promise<void>to match the actual behavior ofsdk.post():'video-conference/jitsi.update-timeout': { POST: (params: { roomId: string }) => Promise<void>; };
🤖 Fix all issues with AI agents
In `@app/definitions/rest/v1/channels.ts`:
- Around line 5-7: The closing brace in the type declaration for
AdaptChannelsEndpoints has inconsistent indentation (an extra tab before the
brace); align the closing brace with the start of the type declaration so it
matches the indentation of "type AdaptChannelsEndpoints<T> = {" and the opening
brace—edit the line containing the trailing brace for AdaptChannelsEndpoints to
remove the extra tab so formatting is consistent with RemoveV1Prefix and the
rest of the file.
In `@app/definitions/rest/v1/customUserStatus.ts`:
- Around line 11-13: The closing brace in the type declaration
AdaptCustomUserStatusEndpoints has extra indentation; adjust the closing brace
for that mapped type to match the opening indentation (remove the double tab so
the brace aligns with the type declaration), ensuring consistent indentation for
the entire type block.
In `@app/definitions/rest/v1/settings.ts`:
- Line 67: The isOauthCustomConfiguration type guard currently always returns
Boolean(config), which accepts any truthy value and doesn't validate required
fields; update isOauthCustomConfiguration to perform real runtime checks for the
OauthCustomConfiguration shape (e.g., verify presence and types of required
properties like clientId, clientSecret, redirectUri, or whichever fields the
OauthCustomConfiguration interface declares) or delete the function entirely if
runtime validation is unnecessary and you rely solely on compile-time types;
reference the isOauthCustomConfiguration function and the
OauthCustomConfiguration type when making the change.
- Around line 69-80: The local declaration of SettingsEndpoints redefines the
transformed endpoint 'settings/:_id' (from
AdaptSettingsEndpoints<RestTypingsSettingsEndpoints>) which can produce a union
if the local GET/POST signatures differ from the upstream; either add a concise
in-line comment above the 'settings/:_id' block stating that this is an
intentional override of the upstream type (mentioning AdaptSettingsEndpoints and
RestTypingsSettingsEndpoints) and why, or confirm and align the local signatures
(GET returning Pick<ISetting,'_id'|'value'> and POST accepting
SettingsUpdateProps) so they exactly match the upstream contract to avoid a
merged union type. Ensure the comment references the symbols SettingsEndpoints,
'settings/:_id', ISetting, and SettingsUpdateProps so future readers know the
override is deliberate and compatible.
In `@app/definitions/rest/v1/teams.ts`:
- Around line 5-7: The closing brace in the generic type declaration for
AdaptTeamsEndpoints has inconsistent indentation (an extra tab); fix the
formatting by aligning the closing brace with the opening "type
AdaptTeamsEndpoints<T> = {" line and the rest of the block so it matches the
style used in channels.ts—adjust the indentation of the line containing "};"
that closes the mapped type using RemoveV1Prefix and the mapped key signature.
♻️ Duplicate comments (1)
app/lib/services/restApi.ts (1)
359-361: Theusers.getPreferencesendpoint does not accept auserIdparameter.This issue was previously identified. The API returns preferences for the authenticated user based on headers only—the
userIdparameter will be ignored. Theas anycast masks an incorrect API call.
🧹 Nitpick comments (4)
app/lib/services/restApi.ts (2)
337-346: Return type loosened toPromise<any>reduces caller type safety.Changing from a typed response to
Promise<any>means callers lose compile-time guarantees on the response shape. If the rest-typings package provides proper response types forsubscriptions.readandsubscriptions.unread, consider using them instead ofany.
965-968: Trackas anycasts for post-migration cleanup.This file has accumulated many
as anycasts (e2e.resetRoomKey,chat.update,users.removeOtherTokens,rooms.invite, etc.) as workarounds during the rest-typings migration. Consider:
- Opening an issue to track these for cleanup
- Adding inline TODOs with issue references
- Contributing missing type definitions upstream to
@rocket.chat/rest-typingsapp/definitions/rest/v1/channels.ts (1)
1-9: Consider centralizingRemoveV1Prefix<T>into a shared utility type.This helper type is duplicated across multiple endpoint definition files (
channels.ts,moderation.ts,commands.ts,teams.ts,customUserStatus.ts, and likely others). Extract it to a shared location (e.g.,app/definitions/rest/v1/helpers.ts) and import where needed.♻️ Suggested shared helper
Create a new file
app/definitions/rest/v1/helpers.ts:export type RemoveV1Prefix<T> = T extends `/v1/${infer Rest}` ? Rest : T; export type AdaptEndpoints<T> = { [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; };Then in each endpoint file:
-type RemoveV1Prefix<T> = T extends `/v1/${infer Rest}` ? Rest : T; - -type AdaptChannelsEndpoints<T> = { - [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; - }; +import type { AdaptEndpoints } from './helpers'; -export type ChannelsEndpoints = AdaptChannelsEndpoints<RestTypingsChannelsEndpoints>; +export type ChannelsEndpoints = AdaptEndpoints<RestTypingsChannelsEndpoints>;app/definitions/rest/v1/e2e.ts (1)
3-7: ExtractRemoveV1Prefixto a shared utilities file to reduce duplication across 23 endpoint modules.
RemoveV1Prefixand the adapter type pattern are identically duplicated across all v1 endpoint files (channels, chat, e2e, rooms, users, etc.). Define these once inapp/definitions/rest/helpers.tsand import as needed.♻️ Suggested refactoring
Create shared helper file:
// app/definitions/rest/helpers.ts export type RemoveV1Prefix<T> = T extends `/v1/${infer Rest}` ? Rest : T; export type AdaptEndpoints<T> = { [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; };Then in each endpoint file:
import type { E2eEndpoints as RestTypingsE2eEndpoints } from '@rocket.chat/rest-typings'; +import type { AdaptEndpoints } from '../helpers'; -type RemoveV1Prefix<T> = T extends `/v1/${infer Rest}` ? Rest : T; - -type AdaptE2eEndpoints<T> = { - [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; -}; - -export type E2eEndpoints = AdaptE2eEndpoints<RestTypingsE2eEndpoints> & { +export type E2eEndpoints = AdaptEndpoints<RestTypingsE2eEndpoints> & {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
app/definitions/rest/v1/autotranslate.tsapp/definitions/rest/v1/channels.tsapp/definitions/rest/v1/chat.tsapp/definitions/rest/v1/commands.tsapp/definitions/rest/v1/customUserStatus.tsapp/definitions/rest/v1/e2e.tsapp/definitions/rest/v1/moderation.tsapp/definitions/rest/v1/settings.tsapp/definitions/rest/v1/teams.tsapp/definitions/rest/v1/videoConference.tsapp/lib/methods/getSlashCommands.tsapp/lib/services/restApi.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/definitions/rest/v1/chat.ts
🧰 Additional context used
🧬 Code graph analysis (6)
app/definitions/rest/v1/customUserStatus.ts (1)
app/definitions/rest/v1/index.ts (1)
Endpoints(25-47)
app/lib/methods/getSlashCommands.ts (1)
app/definitions/ISlashCommand.ts (1)
ISlashCommandResult(12-14)
app/definitions/rest/v1/commands.ts (1)
app/definitions/rest/v1/index.ts (1)
Endpoints(25-47)
app/definitions/rest/v1/autotranslate.ts (1)
app/definitions/rest/v1/index.ts (1)
Endpoints(25-47)
app/lib/services/restApi.ts (1)
app/lib/notifications/index.ts (1)
getDeviceToken(83-83)
app/definitions/rest/v1/settings.ts (1)
app/definitions/ISetting.ts (2)
ISettingColor(102-106)ISetting(20-20)
🔇 Additional comments (17)
app/lib/methods/getSlashCommands.ts (3)
27-28: Reasonable type assertion for improved internal type safety.The typed alias improves type safety within the function. Given that the null/length check on line 22 guards against undefined/empty cases, this is a safe approach.
However, the reliance on
@ts-ignoreat lines 14-15 and 20-21 suggests the SDK endpoint typings don't yet align with the actual API response shape. As this PR aims to use@rocket.chat/rest-typingsas the single source of truth, consider creating a follow-up task to properly type thecommands.listendpoint response so these@ts-ignorecomments can be removed.
30-34: LGTM!Consistent use of the typed
commandsTypedvariable in filter operations. The explicit type annotations on callback parameters (e.g.,i1: ISlashCommandResult) are now technically redundant since TypeScript can infer them from the array type, but they don't hurt readability.
54-54: LGTM!Correctly uses
commandsTypedfor the update lookup, ensuring consistent typing throughout the function.app/definitions/rest/v1/autotranslate.ts (1)
1-15: Type transformation syntax is correct, but endpoint extraction requires verification.The type transformation uses idiomatic TypeScript patterns—conditional mapped types and template literal string manipulation are syntactically sound. The
RemoveV1Prefixutility correctly strips the/v1/prefix from keys, and the overall pipeline structure is well-designed.However, the success of this approach depends on whether the
Endpointstype from@rocket.chat/rest-typingsactually contains keys matching the/v1/autotranslate.${string}pattern. Verify that the rest-typings package exports autotranslate endpoints with this naming convention to ensure the extraction filter produces the expected keys.app/definitions/rest/v1/videoConference.ts (1)
1-7: Clean utility type pattern for adapting upstream types.The
RemoveV1Prefixconditional type and the key remapping inAdaptVideoConferenceEndpointsare well-designed. This approach cleanly strips the/v1/prefix from the canonical rest-typings package while preserving type safety.app/lib/services/restApi.ts (6)
260-260: Acceptable migration workaround, but track for cleanup.The
as anycast here is a reasonable interim solution while integrating rest-typings. Consider adding a TODO comment or tracking issue to revisit these casts once the endpoint definitions are fully aligned.
473-478: LGTM with caveat.The endpoint path cast is a necessary workaround. The explicit return type annotation maintains type safety for callers despite the internal
as any.
599-601: Verifyignoreparameter type forchat.ignoreUserendpoint.The change to
ignore.toString()suggests the rest-typings definition expects a string. Confirm this matches the Rocket.Chat server API expectation—some REST APIs accept boolean query params directly.
997-1002: LGTM!The explicit type annotation
'apn' | 'gcm'improves clarity and ensures type safety for the push token type.
428-429: [Your rewritten review comment text here]
[Exactly ONE classification tag]
818-826: Breaking change:triggerIdis now required.All callers of
runSlashCommandin the codebase already provide this parameter—for example,MessageComposer.tsxgenerates it viagenerateTriggerId(appId)before calling the function. No changes needed.app/definitions/rest/v1/moderation.ts (1)
1-9: LGTM!The type transformation correctly adapts
RestTypingsModerationEndpointsby stripping the/v1/prefix from endpoint keys. The implementation aligns with the broader migration pattern in this PR.app/definitions/rest/v1/commands.ts (1)
1-15: LGTM - extraction approach is appropriate.Since
@rocket.chat/rest-typingsdoesn't exportCommandsEndpointsdirectly, extracting them from the mainEndpointstype using the key filter pattern is the correct approach. The transformation logic is sound.app/definitions/rest/v1/customUserStatus.ts (1)
1-15: LGTM - type logic is correct.The extraction and adaptation pattern correctly filters
custom-user-status.*endpoints and strips the/v1/prefix. Follows the same approach ascommands.tsfor endpoints not directly exported by@rocket.chat/rest-typings.app/definitions/rest/v1/e2e.ts (1)
9-16: LGTM!The adapter pattern correctly transforms the imported
RestTypingsE2eEndpointsby stripping the/v1/prefix, and the intersection type properly extends it with local endpoints that aren't yet in the package. The implementation aligns well with the PR objective of using@rocket.chat/rest-typingsas the source of truth.app/definitions/rest/v1/settings.ts (2)
1-10: Helper types and imports align with refactoring goal.The
RemoveV1PrefixandAdaptSettingsEndpointsutility types correctly strip the/v1/prefix from upstream endpoint keys, enabling seamless integration with the existing codebase's endpoint naming convention. The special case handling for/v1/settings/:_idon line 9 is a clean approach.
18-34: Type guards correctly discriminate settings update variants.The property-based checks (
'execute' in props,'editor' in props && 'value' in props,'value' in props) properly narrow the union type at runtime. The order of checks matters when used in conditionals—ensure callers checkisSettingsUpdatePropsActionsandisSettingsUpdatePropsColorbeforeisSettingsUpdatePropDefaultsince the latter's check is more general.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Fix unsafe type assertions by replacing 'as any' with safer 'as unknown as' casts - Fix getSingleMessage to properly map _id to id field for IMessage interface - Fix getUserPreferences to remove unsupported userId parameter - Fix DiscussionsView to use proper type interface instead of 'as any' - Fix ReadReceiptView to use safer type assertion with documentation - Fix UserNotificationPreferencesView to use safer type assertion - Fix isOauthCustomConfiguration type guard to perform actual validation - Fix indentation issues in channels.ts, customUserStatus.ts, and teams.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/lib/services/restApi.ts`:
- Around line 1084-1087: The function logoutOtherLocations assumes
reduxStore.getState().login.user.id exists; add a guard in logoutOtherLocations
to read const { id } = reduxStore.getState().login.user and if id is falsy
either throw a clear Error or return a rejected Promise (or no-op) before
calling sdk.post('users.removeOtherTokens'), and pass a correctly typed payload
(e.g., { userId: id }) only when id is present to avoid sending undefined to
users.removeOtherTokens.
♻️ Duplicate comments (2)
app/definitions/rest/v1/teams.ts (1)
5-9: Indentation issue on closing brace appears unresolved.The past review flagged inconsistent indentation on line 7 (extra tab before
};). This line was not modified in the current revision, so the fix may not have been applied.🔧 Fix
type AdaptTeamsEndpoints<T> = { [K in keyof T as RemoveV1Prefix<K & string>]: T[K]; - }; +};app/definitions/rest/v1/customUserStatus.ts (1)
9-13: Type transformation logic is correct.The
RemoveV1Prefixconditional type andAdaptCustomUserStatusEndpointsmapped type correctly strip the/v1/prefix from endpoint keys. TheK & stringintersection properly narrows the key type for template literal compatibility.Note: An indentation issue on the closing brace was flagged in a previous review. Please ensure it has been addressed.
🧹 Nitpick comments (6)
app/lib/methods/getSingleMessage.ts (1)
4-5: Consider simplifying the Promise wrapper (optional).The
new Promise(async ...)pattern is redundant since async functions already return Promises. A simpler approach:♻️ Suggested simplification
-const getSingleMessage = (messageId: string): Promise<IMessage> => - new Promise(async (resolve, reject) => { - try { - const result = await getSingleMessageService(messageId); - if (result.success && result.message) { - const { message } = result; - // Map _id to id to match IMessage interface structure - // The API returns messages with _id, but IMessage requires id field - return resolve({ - ...message, - id: message._id - } as unknown as IMessage); - } - return reject(); - } catch (e) { - return reject(e); - } - }); +const getSingleMessage = async (messageId: string): Promise<IMessage> => { + const result = await getSingleMessageService(messageId); + if (result.success && result.message) { + const { message } = result; + // Map _id to id to match IMessage interface structure + // The API returns messages with _id, but IMessage requires id field + return { + ...message, + id: message._id + } as unknown as IMessage; + } + throw new Error('Failed to retrieve message'); +};app/definitions/rest/v1/settings.ts (1)
36-65: Consider tighteningunknownfield types incrementally.The extensive use of
unknownfor many fields is safer thanany, but if the actual shapes of these OAuth configuration fields are known fromLoginServiceConfigurationor backend documentation, consider typing them more precisely in a follow-up. This isn't blocking, but would improve type safety for consumers of this configuration.app/lib/services/restApi.ts (4)
333-346: Consider preserving return type safety.The
Promise<any>return type discards type information. If the two endpoints return different response shapes, consider using a union type or generic to preserve type safety for callers.-): Promise<any> => { +): Promise<{ success: boolean }> => {
489-491: Consider separating query parameters from path.Embedding query parameters in the path string is unconventional. If the SDK supports it, consider passing
enabledDepartmentsOnlyas a params object for clarity and type safety:- sdk.get(`livechat/agents/${uid}/departments?enabledDepartmentsOnly=true` as any); + sdk.get(`livechat/agents/${uid}/departments` as any, { enabledDepartmentsOnly: true });
647-652: Type alignment opportunity.The
as anycasts for bothdataandcustomFieldssuggest the localIProfileParamsdefinition may differ from@rocket.chat/rest-typings. Consider aligning the local type with the package definition to eliminate these casts.
426-435: Consider tracking missing endpoint type definition in a consolidated issue.The
as anycasts on line 430 indicatelivechat/inquiries.returnAsInquirylacks type definitions from@rocket.chat/rest-typings. This pattern is consistent with other newer endpoints in the file (lines 652, 968, 1096). While the file already contains TODO comments for missing definitions elsewhere, consolidating these into a single tracking issue for batch-updating rest-typings when the package is updated would be more maintainable than scattered inline TODOs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
app/definitions/rest/v1/channels.tsapp/definitions/rest/v1/customUserStatus.tsapp/definitions/rest/v1/settings.tsapp/definitions/rest/v1/teams.tsapp/lib/methods/getSingleMessage.tsapp/lib/services/restApi.tsapp/views/DiscussionsView/index.tsxapp/views/ReadReceiptView/index.tsxapp/views/UserNotificationPreferencesView/index.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/views/ReadReceiptView/index.tsx
- app/views/DiscussionsView/index.tsx
- app/views/UserNotificationPreferencesView/index.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
app/lib/methods/getSingleMessage.ts (1)
app/definitions/IMessage.ts (1)
IMessage(147-178)
app/definitions/rest/v1/settings.ts (1)
app/definitions/ISetting.ts (2)
ISettingColor(102-106)ISetting(20-20)
app/definitions/rest/v1/customUserStatus.ts (1)
app/definitions/rest/v1/index.ts (1)
Endpoints(25-47)
🔇 Additional comments (21)
app/definitions/rest/v1/teams.ts (1)
1-4: LGTM!The type-only import and
RemoveV1Prefixutility type are correctly implemented. The conditional template literal type correctly strips the/v1/prefix when present.app/lib/methods/getSingleMessage.ts (1)
8-15: Explicit_id→idmapping addresses the prior type mismatch concern.The change correctly maps the API's
_idfield to theidfield required byIMessage. The spread pattern preserves all other fields while adding the required mapping. Theas unknown as IMessageassertion is acceptable given the external API type constraints during this migration.app/definitions/rest/v1/customUserStatus.ts (2)
1-7: LGTM! Type extraction logic is well-structured.The use of template literal types to filter endpoints matching
/v1/custom-user-status.${string}is an idiomatic approach for extracting a subset of the upstreamEndpointstype.
15-15: Export correctly derives the type from upstream package.This approach ensures
CustomUserStatusEndpointsstays in sync with@rocket.chat/rest-typingswhile adapting the key format for local usage. This aligns well with the PR objective of using the package as the single source of truth.app/definitions/rest/v1/channels.ts (1)
1-9: Clean implementation of the type adaptation pattern.The use of
RemoveV1PrefixandAdaptChannelsEndpointshelper types to derive endpoints from@rocket.chat/rest-typingsis consistent with the PR's goal and follows the same pattern used in other endpoint files.app/definitions/rest/v1/settings.ts (3)
18-34: AI summary is inconsistent with actual code.The AI summary states that runtime type guards (
isSettingsUpdatePropsActions,isSettingsUpdatePropsColor,isSettingsUpdatePropDefault) were removed, but they are clearly present and correctly implemented in the code.
67-72: Type guard now provides meaningful validation.This implementation properly checks for the presence and types of required fields (
_id,loginStyle) and validates theloginStylevalue. This addresses the previous concern about the type guard accepting any truthy value.
74-87: Override documentation added as requested.The comments on lines 81-82 now clearly explain why
settings/:_idis overridden from upstream types. This addresses the previous review concern about documenting the intentional override.Note that the intersection type semantics will merge both the upstream and local type definitions for
settings/:_id. As long as the local types (ISetting,SettingsUpdateProps) remain compatible with the actual API responses, this approach maintains type safety while preserving codebase compatibility.app/lib/services/restApi.ts (13)
260-260: Acceptable cast for conditional payload structure.The
as anycast here accommodates the varyingparamsshape based on server version and channel type. This is a reasonable workaround during the rest-typings migration.
271-277: LGTM!Explicitly converting
nulltoundefinedvia??ensures the payload conforms to expected typing wherejoinCodeshould bestring | undefinedrather thanstring | null.
359-362: LGTM - past review concern addressed.The
userIdparameter has been correctly removed, aligning with the Rocket.Chat API documentation whereusers.getPreferencesreturns preferences for the authenticated user based on headers only.
474-479: Acceptable cast for newer endpoint.The
livechat/config/routingendpoint (RC 7.11.0+) may not be in@rocket.chat/rest-typingsyet. The cast is appropriate for forward compatibility.
600-602: LGTM!Converting the boolean
ignoreto a string aligns with REST API query parameter conventions where boolean values are typically passed as'true'/'false'strings.
658-660: Acceptable migration cast.The
as anycast fornotificationsparameter follows the same pattern. Consider aligningIRoomNotificationswith the rest-typings definition in a follow-up.
697-704: LGTM!The
as anycast for the sort parameter accommodates the MongoDB-style sort format (-1for descending) which may not match the strict typing in rest-typings.
798-810: LGTM!The params object is well-typed locally with explicit optional
texthandling. Theas anycast bridges the gap with rest-typings definitions.
819-827: LGTM!The
triggerIdparameter is now explicitly typed as a requiredstring, improving type safety for slash command execution.
971-992: LGTM!The
editMessagefunction correctly handles both encrypted (content) and non-encrypted (text) message updates. Theas anycasts accommodate the different payload shapes required bychat.update.
994-1012: LGTM!Narrowing
typeto'apn' | 'gcm'union improves type safety by restricting to the only valid push notification types for iOS and Android respectively.
1096-1096: Acceptable cast for missing endpoint definition.The
rooms.inviteendpoint may not be defined in@rocket.chat/rest-typings. The action parameter is well-typed as a union. Consider tracking this for future type alignment.
966-969: Document the minimum server version for this endpoint.The
RC ?comment indicates uncertainty about whene2e.resetRoomKeywas introduced. Unlike other e2e endpoints in this file that document specific versions (RC 0.70.0, 2.2.0, 5.5), this one lacks version information. Investigate the Rocket.Chat server documentation or changelog to identify and document the minimum supported version.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…necessary comments - Add guard to check if user id exists before calling users.removeOtherTokens - Return rejected promise with error message if user is not logged in - Improves error handling and code cleanliness
Proposed changes
This PR refactors the app to use @rocket.chat/rest-typings as the single source of truth for REST API type definitions.
Issue(s)
Closes #6232
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.